OCPBUGS-9037, OCPBUGS-64565: Use cluster wildcard certificate for ingress canary #1155
Conversation
openshift-ci-robot
added
jira/severity-important
|
@rfredette: This pull request references Jira Issue OCPBUGS-9037, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
rfredette
force-pushed
the
ocpbugs-9037-use-cluster-wildcard
branch
from
4f7f29d to
a7792fa
Compare
|
test failures appear unrelated. |
Miciah
added
priority/backlog
|
/assign @Miciah |
|
/retest-required |
There was a problem hiding this comment.
@rfredette I've had these comments pending for some time. Please let me know if I've misunderstood the assignment here, but in some places it looks like we are just reusing the default cert. Was that the plan?
| apiVersion: v1 | ||
| metadata: | ||
| metadata: {} | ||
| # name and namespace are set at runtime. |
There was a problem hiding this comment.
nit:
| # name and namespace are set at runtime. | |
| # metadata values are set at runtime. |
| secretName, err := r.canarySecretName(controller.CanaryDaemonSetName().Namespace) | ||
| if err != nil { | ||
| return false, nil, err | ||
| } |
There was a problem hiding this comment.
Why not get the secretName within desiredCanaryDaemonSet? Then you don't need to pass it in as a parameter.
There was a problem hiding this comment.
In order to enable desiredCanaryDaemonSet to get the secret, we would need to pass the client to desiredCanaryDaemonSet. Passing the client to desiredCanaryDaemonSet would go against our established pattern that ensureFoo gets the necessary inputs for desiredFoo and passes those specific inputs to desiredFoo. Passing the client would also mean we would be adding a parameter to desiredCanaryDaemonSet anyway, so it wouldn't really have any benefit over the current change.
| daemonset.Spec.Template.Spec.Containers[0].Image = canaryImage | ||
| daemonset.Spec.Template.Spec.Containers[0].Command = []string{"ingress-operator", CanaryHealthcheckCommand} | ||
|
|
||
| daemonset.Spec.Template.Spec.Volumes[0].Secret.SecretName = secretName |
There was a problem hiding this comment.
It would be better to call canarySecretName right here instead of in the ensure function, and then passing it in here.
| daemonset.Spec.Template.Spec.Volumes[0].Secret.SecretName = secretName | |
| daemonset.Spec.Template.Spec.Volumes[0].Secret.SecretName, err := canarySecretName(name.Namespace) |
There was a problem hiding this comment.
And then you could test the secret generation in Test_desiredCanaryDaemonSet instead of writing a new test for ensureCanaryDaemonSet.
| UID: daemonset.UID, | ||
| Controller: &trueVar, | ||
| } | ||
| if _, err := r.ensureDefaultCertificateForIngress(ca, "openshift-ingress-canary", canaryRef, ingress); err != nil { |
There was a problem hiding this comment.
Doesn't there need to be a different function to ensure a canary cert rather than ensure a default cert? Does this ensure the correct cert?
There was a problem hiding this comment.
The intent is that the canary application should use the default IngressController's default certificate. This way, as long as the default IngressController has a correctly configured default certificate, so too will the canary application. Because #978 changed the canary application to use a TLS passthrough route, the only way to have the canary application use the default IngressController's default certificate is to copy that certificate to the canary application's namespace and configure the application to use that copy of the certificate.
If I understand correctly, ensureDefaultCertificateForIngress actually generates a new server certificate, using the existing CA certificate, so this logic doesn't quite implement the intent. We could use r.ensureDefaultCertificateForIngress(ca, ingress.Namespace, ref, ingress) (note the namespace) to get the existing certificate (or create it if it's missing), but it seems simpler just to do a Get from ingress.Namespace and then Create in "openshift-ingress-canary".
|
/restest-required |
|
Failure in infra: /test e2e-hypershift |
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
openshift-ci
bot
added
the
lifecycle/stale
|
Stale issues rot after 30d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle rotten |
openshift-ci
bot
added
lifecycle/rotten
|
Rotten issues close after 30d of inactivity. Reopen the issue by commenting /close |
|
@openshift-bot: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@rfredette: This pull request references Jira Issue OCPBUGS-9037. The bug has been updated to no longer refer to the pull request using the external bug tracker. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/reopen |
|
@rfredette: Reopened this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@rfredette: This pull request references Jira Issue OCPBUGS-9037, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
rfredette
force-pushed
the
ocpbugs-9037-use-cluster-wildcard
branch
from
a7792fa to
768f6b8
Compare
|
/remove-lifecycle rotten |
openshift-ci
bot
removed
the
lifecycle/rotten
| log.Info("canary daemonset not found; will retry default cert sync") | ||
| result.RequeueAfter = 5 * time.Second | ||
| } else { | ||
| errs = append(errs, fmt.Errorf("failed to get daemonset: %v", err)) |
There was a problem hiding this comment.
Use %w when wrapping errors, on line 120 and on line 132.
| errs = append(errs, fmt.Errorf("failed to ensure default cert for %s: %v", ingress.Name, err)) | ||
| } | ||
| if ingress.Name == manifests.DefaultIngressControllerName { | ||
| log.Info("ensuring canary certificate") |
There was a problem hiding this comment.
| log.Info("ensuring canary certificate") | |
| log.Info("Ensuring canary certificate") |
| if _, err := r.ensureDefaultCertificateForIngress(ca, deployment.Namespace, deploymentRef, ingress); err != nil { | ||
| errs = append(errs, fmt.Errorf("failed to ensure default cert for %s: %v", ingress.Name, err)) | ||
| } | ||
| if ingress.Name == manifests.DefaultIngressControllerName { |
There was a problem hiding this comment.
Please add a comment to explain why this logic only applies to the "default" IngressController. Namely, the canary check is specifically for verifying that the default IngressController is ready.
rfredette
changed the title
openshift-ci-robot
added
jira/severity-critical
|
@rfredette: This pull request references Jira Issue OCPBUGS-9037, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. This pull request references Jira Issue OCPBUGS-64565, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@Miciah: This pull request references Jira Issue OCPBUGS-9037, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: This pull request references Jira Issue OCPBUGS-64565, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/retest e2e-aws-operator |
|
@candita: The The following commands are available to trigger optional jobs: Use In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/retest |
|
The bug OCPBUGS-9037 mentioned console health check as well, seems this fix is just for ingress canary, I'm wondering how to fix the console route ? |
|
@rfredette: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
6 participants
Utilize the existing ingress controller certificate management controller to also manage the certificate for the ingress canary, and use that certificate when serving the canary endpoint.